Prepare tests for downstream - #334
Conversation
Reviewer's GuidePrepares the test utilities and Makefile for use from downstream projects by making key helpers public, parameterizing paths, refactoring VM backends through a shared node interface, and consolidating logging and TLS handling. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="test_utils/src/virt/mod.rs" line_range="172-177" />
<code_context>
}
}
+#[async_trait::async_trait]
+impl NodeBackend for AzureBackend {
+ async fn ssh_exec(&self, command: &str) -> Result<String> {
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `NodeBackend::get_root_key` UUID extraction and error paths
The updated `NodeBackend::get_root_key` centralizes UUID extraction and secret lookup, but key edge cases aren’t covered by tests: empty UUID output, SSH/`cryptsetup` failure, missing secrets, and secrets without a `root` key. Please add tests that mock `ssh_exec` and the Kubernetes client to cover: (1) a successful case where `root` is returned; (2) failure when the UUID output is empty; and (3) failure when the secret or `root` key is missing. A test-only backend is fine as long as these behaviors are clearly defined for downstream attestation tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| #[async_trait::async_trait] | ||
| #[auto_impl::auto_impl(Box)] | ||
| pub trait VmBackend: Send + Sync { | ||
| async fn create_vm(&self) -> Result<()>; | ||
| async fn wait_for_running(&self, timeout_secs: u64) -> Result<()>; | ||
| pub trait NodeBackend: Send + Sync { | ||
| async fn ssh_exec(&self, command: &str) -> Result<String>; | ||
| async fn get_root_key(&self) -> Result<Option<Vec<u8>>>; | ||
| async fn cleanup(&self) -> Result<()>; | ||
|
|
||
| async fn get_boot_id(&self) -> Result<String> { | ||
| let id = self.ssh_exec("cat /proc/sys/kernel/random/boot_id").await?; | ||
| Ok(id.trim().to_string()) | ||
| } | ||
| async fn get_root_key(&self, client: Client, namespace: &str) -> Result<Option<Vec<u8>>> { |
There was a problem hiding this comment.
suggestion (testing): Add tests for NodeBackend::get_root_key UUID extraction and error paths
The updated NodeBackend::get_root_key centralizes UUID extraction and secret lookup, but key edge cases aren’t covered by tests: empty UUID output, SSH/cryptsetup failure, missing secrets, and secrets without a root key. Please add tests that mock ssh_exec and the Kubernetes client to cover: (1) a successful case where root is returned; (2) failure when the UUID output is empty; and (3) failure when the secret or root key is missing. A test-only backend is fine as long as these behaviors are clearly defined for downstream attestation tests.
74cdb97 to
0fc653c
Compare
|
nit: One of your commits name is |
0fc653c to
5afd0ca
Compare
Jakob-Naucke
left a comment
Jakob-Naucke
left a comment
There was a problem hiding this comment.
nit: One of your commits name is
[test_utils/virt: s/s(sh_exec)/\1/]. is this intentional?
Yes, it's the sed syntax for replacing ssh_exec with sh_exec
Right :)
NVM, looks like the only disallowed character in a commit message is a NULL byte. |
5afd0ca to
dde46ba
Compare
|
/test azure-integration-test |
not relative to working directory, for inclusion from other Makefiles Signed-off-by: Jakob Naucke <jnaucke@redhat.com> Assisted-by: AI
for use from other crates Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
for use from other crates Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
to a new `NodeBackend` trait, namely, `ssh_exec`, `get_root_key`, and KubeVirt's implementation of `verify_encrypted_root` (which Azure overrides), for sharing in node-level tests. Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
It really doesn't do SSH at all Signed-off-by: Jakob Naucke <jnaucke@redhat.com>
dde46ba to
566f553
Compare
|
ugh, lief. I also didn't know you'd have the test with your PR once you'd run it /test azure-integration-test |
|
still lief, but it seems better now /test azure-integration-test |
|
still /test azure-integration-test |
|
@SpaceFace02 I'd live without passing Azure tests, does the comment LGTY? |
|
/test azure-integration-test |
|
@Jakob-Naucke: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Seemed like a delayed Trustee pod restart, so this should improve with #248 /test azure-integration-test |
60125df
into
trusted-execution-clusters:main
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alicefr, Jakob-Naucke, SpaceFace02 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
..get_encoded_root_pem,get_envpublicTestContextclonableTestContext.infowhen possible to save linesPreview of downstream test to use these changes: https://github.com/Jakob-Naucke/confidential-clusters-operator/tree/scale-test
Summary by Sourcery
Prepare test utilities and build tooling to be reusable from downstream projects and support multiple virtualization backends.
New Features:
Enhancements:
Build:
Tests: